Simplify relationship check for conditional type on target side #46429
Conversation
typescript-bot
added
the
For Milestone Bug
|
I should mention that with this PR, the check time for this repro drops from ~10s to about 0.4s. |
ahejlsberg
requested review from
weswigham,
andrewbranch,
armanio123 and
DanielRosenwasser
|
@typescript-bot test this |
|
Heya @andrewbranch, I've started to run the inline community code test suite on this PR at 75ab347. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the extended test suite on this PR at 75ab347. You can monitor the build here. |
|
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 75ab347. You can monitor the build here. |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 75ab347. You can monitor the build here. |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 75ab347. You can monitor the build here. Update: The results are in! |
|
Hurray, I helped find something maybe useful! |
|
@andrewbranch |
| @@ -48,7 +55,13 @@ tests/cases/compiler/conditionalTypeAssignabilityWhenDeferred.ts(116,3): error T | |||
| function f<T>(t: T) { | |||
| var x: T | null = Math.random() > 0.5 ? null : t; | |||
| onlyNullablePlease(x); // should work | |||
| ~ | |||
| !!! error TS2345: Argument of type 'T | null' is not assignable to parameter of type 'null extends T | null ? any : never'. | |||
There was a problem hiding this comment.
This one is interesting - is this actually a distributive conditional type? Why is it deferred?
There was a problem hiding this comment.
We always defer if either of the check and extends types contains generics. This is somewhat of a blunt instrument, and we could possibly do better in cases like this.
There was a problem hiding this comment.
And by "contains generics" I mean we defer when isGenericType returns true for either of the check and extends types.
This is impossible in the general case - any condition with an |
|
@andrewbranch Here they are:Comparison Report - main..46429
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 75ab347. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 75ab347. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the tarball bundle task on this PR at 75ab347. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 75ab347. You can monitor the build here. |
Agreed, and so be it. But it does seem like there are classes of cases where we ought to do better in conditional type construction. Both of the test cases that revert back to failing with this PR seem quite trivial--for example, checking if the check and extends types are identical, or if the extends type is a union that contains the check type, would cause us to resolve the conditional type to the true case. |
|
Hey @ahejlsberg, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
I'll reiterate that I'm with ignoring my repro since I probably made nonsense out of the logic while tinkering with it. Obviously, never being slow is the ideal, but it's more important to focus on code people might actually write. Alternatively, we could call this good and file a separate bug to go investigate that repro. |
|
Please, no more plot twists, my heart can't handle it anymore. |
|
Yeah, if the real world code is fast then I’m happy. Just wanted to make sure it was checked against the latest changes. |
|
is there an example of a type that is no longer allowed as of this PR? the typescript 4.5 blog post mentions this PR
but i can't seem to find a minimal example of what this means exactly |
|
@DetachHead good news (??), we found one: #47127 (comment) |
This is a BAD news for me. I deeply dependency this feature. Because of this breaking change, I cant update TypeScript, it's so bad for me, could you add a compiler option to support it as a migrate plan? |
…osoft#46429) * Simplify relationship check for conditional type on target side * Accept new baselines * Better support for non-distribution-dependent types * Accept new API baselines * Accept new baselines
This PR simplifies our relationship checking logic for conditional types on the target side. With this PR, a source type
Sis assignable to a target typeA extends B ? C : DwhenAis never referenced inCorD, andBhas noinferpositions, andSis assignable toC(except ifAnever assignable toBfor any instantiation), andSis assignable toD(except ifAis always assignable toBfor any instantiation).The PR revises some of the logic that was added in #30639, specifically the logic that determines if
Ais referenced inCorDin a distributive conditional type. There are no baseline changes as a result of this PR, but it fixes the performance degradation reported in #44851.Fixes #44851.